Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using dict as an OrderedDict and allowed using dict as an ordered type in setuptools.dist.check_requirements #4575

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Aug 17, 2024

Summary of changes

Allows using dict as an ordered type in setuptools.dist.check_requirements
#4574 Gave me the idea for this PR

Pull Request Checklist

@@ -52,17 +70,17 @@ def check_importable(dist, attr, value):
) from e


def assert_string_list(dist, attr, value):
def assert_string_list(dist, attr: str, value: _OrderedIterable):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more like assert_ordered_iterable now. I also realize that whilst this is all technically type-safe, we may not want to allow dict for a user-facing reason:
If a user is told they can use a dict here, they're likely going to expect that it means they can use a nested value of some sort, not a "dict with unused values". I also think that forcing a user to coerce their Iterable into a tuple or list in their config file so niche it should be fine.

Copy link
Contributor Author

@Avasam Avasam Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up reverting a lot of this, keeping strict tuple | list checks, and updating messages in #4578 instead.

@Avasam Avasam force-pushed the Allow-dict-as-ordered-set branch 2 times, most recently from e2d5654 to 0cfaff7 Compare August 18, 2024 05:04
@Avasam Avasam changed the title Allow using dict where an ordered iterable is expected Allows using dict as an ordered type in setuptools.dist.check_requirements Aug 19, 2024
"""Verify that install_requires is a valid requirements list"""
try:
list(_reqs.parse(value))
if isinstance(value, (dict, set)):
if isinstance(value, set):
Copy link
Contributor Author

@Avasam Avasam Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it curious that this is the only function in this file checking for a "not set" rather than a "tuple or list". Is there any expectation to allow working with any iterable other than tuple or list? Basically the same question I had in #4575 (comment)

I also realize that whilst this is all technically type-safe, we may not want to allow dict for a user-facing reason: If a user is told they can use a dict here, they're likely going to expect that it means they can use a nested value of some sort, not a "dict with unused values". I also think that forcing a user to coerce their Iterable into a tuple or list in their config file so niche it should be fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I try not to be overly-prescriptive about input types. For example, accept Iterable or Sequence so as not to unnecessarily constrain the user. Probably only tuple or list are used commonly, but I'd be open to allowing users to pass in any ordered object with an __iter__.

In this case, we want to assert that whatever value was passed is ordered. The isinstance(, set) check is a poor approximation of that, and it's there mainly as a rough check that the user didn't make a silly mistake.

@Avasam Avasam changed the title Allows using dict as an ordered type in setuptools.dist.check_requirements Using dict as an OrderedDict and allowed using dict as an ordered type in setuptools.dist.check_requirements Aug 20, 2024
"""Verify that install_requires is a valid requirements list"""
try:
list(_reqs.parse(value))
if isinstance(value, (dict, set)):
if isinstance(value, set):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I try not to be overly-prescriptive about input types. For example, accept Iterable or Sequence so as not to unnecessarily constrain the user. Probably only tuple or list are used commonly, but I'd be open to allowing users to pass in any ordered object with an __iter__.

In this case, we want to assert that whatever value was passed is ordered. The isinstance(, set) check is a poor approximation of that, and it's there mainly as a rough check that the user didn't make a silly mistake.

Comment on lines +142 to +143
@overload
def check_requirements(dist, attr: str, value: set) -> NoReturn: ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this at all. We don't want to be declaring that set is valid, because this function is specifically designed to prevent set. I would normally say let's just drop the imperative check and let users rely on mypy to check their types, but I'm guessing it's unlikely that anyone is type-checking their setup.py. Also, I'm unsure if there are other code paths. For example, can a user supply a set using pyproject.toml? It seems not.

So maybe the best thing would be to drop this override and suppress mypy errors/warnings about its usage.

Copy link
Contributor Author

@Avasam Avasam Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to be declaring that set is valid

That's what this overload is doing, preventing the use of set in check_requirements by indicating it'll never return (ie it'll always raise or exit the program).

There's some cases where it doesn't help (like if check_requirement is the last statement of a block), but without it set was already always valid anyway because it matches _StrOrIter.

One could also use the static @deprecated decorator on that overload to further re-enforce and message the user, but that's a bit cheesy (and wouldn't be clean as it'd require a runtime typing_extension import or alias)

Finally as per https://github.com/pypa/setuptools/pull/4575/files#r1733535055 , we can instead try to restrict the type of value further. For instance Sequence[str], which already matches a type of nested strings incidentally, and would prevent passing dict or set.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. As long as IDEs aren't tempted to suggest passing a set because of this annotation, I'm fine with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. As long as IDEs aren't tempted to suggest passing a set because of this annotation, I'm fine with it.

I just left home, but later I can take a look and show you what it looks like in VSCode/Pylance (especially when it comes to intellisense suggestions). As to not make assumptions here.

@overload
def check_requirements(dist, attr: str, value: set) -> NoReturn: ...
@overload
def check_requirements(dist, attr: str, value: _StrOrIter) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think _StrOrIter is wrong here. An Iterable could be an Iterator that can be exhausted after the first _reqs.parse(). What we really want here is an OrderedSequence, which might be defined as every Sequence that's not unordered. I'm not sure if it's possible to define that in mypy, so an approximation is fine (maybe even _StrOrIter).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants